Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fit bitmask overflow initial dirty value #4507

Merged
merged 2 commits into from
Mar 5, 2020
Merged

fit bitmask overflow initial dirty value #4507

merged 2 commits into from
Mar 5, 2020

Conversation

tanhauhau
Copy link
Member

@tanhauhau tanhauhau commented Mar 5, 2020 •
edited
Loading

Fixes #4263

the initial dirty value needs to be depend on whether bitmask context overflow

@tanhauhau tanhauhau force-pushed the tanhauhau/bitmask-overflow-if branch from c7daef7 to 41a23a1 Compare March 5, 2020 01:06
@Conduitry Conduitry merged commit a829122 into sveltejs:master Mar 5, 2020
@Conduitry
Copy link
Member

Well GitHub apparently also attributed this one to me 🙁

@Conduitry
Copy link
Member

I've reopened the issue because, disconcertingly, the tests pass if you run them with npm test but bitmask-overflow-if fails with PUBLISH=true npm test which tests against the actual bundle that would be published. 😱 I have no idea.

@Conduitry
Copy link
Member

The compiler has different output in this test depending on whether it was build with PUBLISH=true. Without it, it generates current_block_type_index$ = select_block_type$(ctx, [-1]); - and in publish mode, it generates current_block_type_index$ = select_block_type$(ctx, -1);.

In the production/PUBLISH build, the method is compiled from

get_initial_dirty_bit() {
	const _this = this;
	// TODO: context-overflow make it less gross

	const val = x`-1`;
	return {
		...val,
		elements: [val],
		get type() {
			return _this.renderer.context_overflow ? 'ArrayExpression' : 'UnaryExpression';
		},
	};
}

to

get_initial_dirty_bit() {
	const _this = this;
	// TODO: context-overflow make it less gross
	const val = x `-1`;
	return Object.assign({}, val, { elements: [val], get type() {
					return _this.renderer.context_overflow ? 'ArrayExpression' : 'UnaryExpression';
			} });
}

i.e., the spread is compiled away.

According to some tests I just did, apparently Object.assign() will aggressively evaluate the getter, while the spread lets it remain lazy. Holy shit.

@tanhauhau tanhauhau mentioned this pull request Mar 5, 2020
Merged
@tanhauhau tanhauhau deleted the tanhauhau/bitmask-overflow-if branch March 5, 2020 02:13
@tanhauhau
Copy link
Member Author

oh my.. we should really look into fixing those lazy code...
anyway, i have a patch for it temporarily #4508

hontas added a commit to hontas/svelte that referenced this pull request Mar 6, 2020
@hontas
Merge branch 'master' of https://github.com/sveltejs/svelte
7d9ff14 
* 'master' of https://github.com/sveltejs/svelte: (137 commits)
  -> v3.19.2
  fix lazy code breaks in build
  fit bitmask overflow initial dirty value in 'if' blocks (sveltejs#4507)
  add dev runtime warning for unknown slot names (sveltejs#4501)
  fix render fallback slot content due to whitespace (sveltejs#4500)
  docs: describe falsy and nullish attribute behavior (sveltejs#4498)
  in spread, distinguish never-updating and always-updating props (sveltejs#4487)
  chore: more specific typings, and add README note about Yarn (sveltejs#4483)
  update changelog
  check for unknown props even if component doesn't have writable props (sveltejs#4454)
  Bump codecov from 3.5.0 to 3.6.5 (sveltejs#4433)
  fix bitmask overflow for slot (sveltejs#4485)
  mark module variables as mutated or reassigned (sveltejs#4469)
  docs: referenced_from_script var value (sveltejs#4486)
  docs: clarify default prop behaviour (sveltejs#4460)
  site: turn fancybutton into custombutton (sveltejs#4476)
  update changelog
  exclude global variables from $capture_state (sveltejs#4475)
  -> v3.19.1
  don't treat $$-names as stores during invalidation (sveltejs#4453)
  ...
taylorzane pushed a commit to taylorzane/svelte that referenced this pull request Dec 17, 2020
@Conduitry @taylorzane
fit bitmask overflow initial dirty value in 'if' blocks (sveltejs#4507)
4c4a41f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong {#if} {:else} with context_overflow (this.context.length > 31)
2 participants
@tanhauhau @Conduitry